fix(core): harden scan durability and idempotency (#303) - #325

Reviewed all 28 files end to end: the five migrations, the lease and fencing layer in api/models/finding.py, the worker loop, the new enrichment worker, the scan routes, observability, and the NVD client. Also ran the suite locally against a scratch Postgres with the migrations applied from base to head.

The lease and fencing design is correct and applied consistently. The fault-injection tests are thorough and map to every acceptance criterion in #303. Two functional gaps should be resolved before merge, plus a few smaller items.

Must fix

1. Terminally failed enrichment jobs are unrecoverable. After 3 attempts fail_enrichment_job marks the job failed. From there POST /api/scans//enrich returns the dead job via ON CONFLICT DO NOTHING, claim_next_enrichment_job only picks pending, and recover_stale_enrichment_jobs only handles expired running leases. So a scan whose enrichment exhausts its retries is stuck unless someone edits the DB by hand. This is a regression from the previous thread-based path, where a re-POST simply worked. Please reset a terminal failed job back to pending on enqueue, or return an explicit 409 telling the operator.

2. rule_evaluations is dead in production. The migration, unique constraint, the save_scan upsert, and the tests all exist, but nothing populates it: scanner/engine.py run_scan returns no evaluations key and no production code emits one. Either wire the engine to emit evaluations, or scope this explicitly as storage-only for now. As written, the #303 evaluations criterion looks met but is not observable in production.

Should fix

3. uq_scans_one_active_per_subscription can leave an INVALID index. If a deployment already has two or more active (pending/running) scans for one subscription, CREATE UNIQUE INDEX CONCURRENTLY fails and leaves an invalid index behind silently. Add a dedupe/cleanup note to the deployment-order doc, or a cleanup step in the migration before the index is created.

4. The enrichment fixture assumes the pending queue is empty. This is a general test-isolation issue, not an environment quirk. claim_next_pending_scan claims the oldest pending scan, but the fixture assumes it claims the scan it just created. Any developer who sets both DATABASE_URL and AZURE_SUBSCRIPTION_ID (common when developing against a real local Postgres plus Azure) and runs the full suite will hit this: the pre-existing role tests in test_auth.py admit a real pending scan, and the enrichment fixture then claims that older row instead of its own scan, so save_scan correctly raises LostLease. Make the fixture robust by truncating scans/enrichment_jobs in setup, or by asserting on the claimed scan_id.

Nits

5. docs/api-reference.md is not updated for Idempotency-Key, the 409/429 responses, the 200 replay response, and the enrich job_id response.
6. The new env vars OPENSHIELD_MAX_SCANS_PER_SUBSCRIPTION_PER_HOUR, SCAN_LEASE_SECONDS, and SCAN_HEARTBEAT_SECONDS are missing from .env.example.
7. worker_heartbeats grows unbounded: one row per worker restart, never cleaned.
8. /metrics now runs full DB aggregates on every scrape with no caching. Fine at current scale, worth a note.
9. Enrichment jobs run serially ahead of scans in the same worker loop. Worth documenting as a throughput characteristic.

What looks good

Fencing is correct and applied uniformly on every authoritative write. The fault-injection suite is the most thorough in the repo and covers every #303 acceptance criterion. The deployment-order doc is honest about the migrate-then-run-workers constraint. Error sanitization is preserved. NVD pagination (resultsPerPage=2000, following totalResults, bounded retries with 429 backoff) is done properly. Metrics are bounded-cardinality.

This is careful work — the connection-lifecycle rework (discard-and-reacquire on an aborted/unknown-status transaction instead of trusting a poisoned connection), the stable_finding_key() identity hash excluding presentation fields so retries update rather than duplicate, and the legacy:<id> backfill for existing rows before adding the unique index (with CREATE INDEX CONCURRENTLY in an autocommit block, so it doesn't lock writes) are all the right calls. CI is green.

One real blocker before this can merge, not about the code itself: this PR's first migration (e4f7a9b2c6d8) forks off d8e4f6a1b2c3, same as #310's 3a76ff935bf6 — both currently share that parent, so if both land as-is alembic heads ends up with two heads. Whichever of #310/#325 merges second needs to rebase and repoint its down_revision, same as the #308/#310 fork we resolved earlier. Given this PR also touches api/models/finding.py/scanner/worker.py/api/routes/scans.py — the same files #310 rewrites — that rebase is going to be a real one, not just a migration-pointer fix. Worth coordinating merge order with #310 explicitly before either goes in.

Requesting changes only for the migration fork — nothing else jumped out as wrong in what I read.